[RTNR] Pass binary data to RTNR #5544

MingJenTai · 2022-03-15T05:09:30Z

This PR allows the host to pass binary data to RTNR

lgirdwood

Need more detail in commit message.

cujomalainey · 2022-03-15T19:54:31Z

src/audio/rtnr/rtnr.c

+	switch (cdata->data->type) {
+	case SOF_RTNR_CONFIG:
+		comp_dbg(dev, "rtnr_set_bin_data(): SOF_RTNR_CONFIG");
+		ret = comp_data_blob_set_cmd(cd->model_handler, cdata);


@lgirdwood can two parameters be passed at the same time? Just wondering if the same model handler can be reused or if they have to be separate

Currently the model_handler is used for settings only( sample rate/ on&off ). For other data we would like to modify the internal data in RTNR directly.

@jsarha is updated this API at, @jsarha can we support multiple large blobs per module/component ?
@MingJenTai I think the data would have to be serialized so that we only send one at a time from the host, @jsarha pls correct me if I'm wrong.

It won't work as such, since the handler only maintains one blob per com_data_blob_handler. I think the easiest would adding multiple handlers to the comp_data, one for each blob, and and select the correct one based on component specific type.

src/audio/rtnr/rtnr.c

cujomalainey · 2022-03-15T19:54:45Z

src/audio/rtnr/rtnr.c

+		comp_dbg(dev, "rtnr_set_bin_data(): SOF_RTNR_DATA_2");
+		ret = RTKMA_API_Set_Data_2(cd->rtk_agl,
+									cdata->data->data,
+									cdata->msg_index,


indentation

src/audio/rtnr/rtnr.c

cujomalainey · 2022-03-15T19:56:25Z

src/audio/rtnr/rtnr.c

@@ -46,6 +50,16 @@ struct rtnr_func_map {
 	rtnr_func func; /**< processing function */
 };

+
+struct param_data_hdr {


is this an additional header? I am confused why you need this

Yes there will be different types of binary data for RTNR. We use this as an identification.

isnt this just a duplicate of our existing header?

Not exactly. The additional magic number could tell us if the data is actually created by Realtek. And with the type identifier we could call different API for different data type.

Ok, but a type does not define the magic inside it. Also why do you need the additional magic? Can't you tell by type field

The magic number will be checked inside RTNR library for an additional verification.

@lgirdwood thoughts?

@cujomalainey param_data_hdr has been removed from wrapper code. It won't be used there. : )

MingJenTai · 2022-03-18T06:23:35Z

Just force pushed a commit to reflect the change of the interface of RTKMA_API_Prepare(). A new build of RTNR library will be provided next week. Thanks.

cujomalainey · 2022-03-20T00:14:47Z

I am of the opinion we should reject this PR in favour of the new single blob mode in #5509 which seems to be safer as it's tied to component state and fuzzable

lgirdwood

I think the data_1/2 usage is confusing, can we clear this up. Thanks

lgirdwood · 2022-03-21T15:32:44Z

src/include/sof/audio/rtnr/rtklib/include/RTK_MA_API.h

+						unsigned int msg_index,
+						unsigned int num_elms,
+						unsigned int elems_remaining);
+


When should we use 1 and when should we use 2 ? can the be better named so users can understand the usage better. I would also put some comments explaining the difference and usage.

@cujomalainey @lgirdwood

Thanks for the review. We'll combine those 2 APIs into 1 to avoid confusion.

Another question is what's the best practice to send binary configuration file to sof component during system startup? The config is about 2KB, and should only be set once.

We've been using sof-ctl which works well, but the same configuration will be sent every time before recording. Besides, using sof-ctl might not be a proper way for final product.

We've also tried using cset-tlv to send config just like the control blobs for the UI. However the config is much larger than control blobs and I have no success during my experiments. Is there file size limit for this?

Thanks.

Just force-pushed a commit to simplify the Set_Data API.

There could be different types of the data, but yes they should be handled internally in library.

Thanks!

You could have a the config data as a static data block as part of the RTNT. This would be build into the DATA section and could be used by default ?

Yes currently there is a default config stored in the static table inside the firmware. The proposed method provides a way to overwrite the default config so that we don't have to re-build firmware each time when a new config is needed.

Using my PR #5509 you could use the new comp_data_blob_handler_new_ext() with single_blob = true and tap into alloc() and free() callbacks to return always the pointer to the static buffer (and return an error if the requested size is bigger than the buffer).

With this approach you should keep using the comp_data_blob_handler and friends mostly the way the other components do. The IPC fragment aggregating would be handled by data_blob.c functions. Just before processing call you should check with comp_is_current_data_blob_valid() if the configuration buffer is currently valid, or if it is in the middle of an update.

This commit allows binary data to be passed to RTNR. There could be different types of the data, and those data will update the internal data of RTNR directly. Signed-off-by: Ming Jen Tai <[email protected]>

lgirdwood

@jsarha any comments for the blob loader ?

lgirdwood · 2022-03-30T13:14:45Z

src/include/user/rtnr.h

@@ -26,6 +26,7 @@ struct sof_rtnr_config {
 	uint32_t reserved[4];

 	struct sof_rtnr_params params;
+	int32_t data[];


Wont you need a size here to tell client code how big data[] is ? or maybe this in fully handled in blob API @jsarha please comment here.

The data is with variable length. Actual size is stored in the size flag.

jsarha · 2022-03-31T12:17:25Z

src/audio/rtnr/rtnr.c

 	int ret = 0;

 	switch (cdata->cmd) {
 	case SOF_CTRL_CMD_BINARY:
 		comp_info(dev, "rtnr_cmd_set_data(), SOF_CTRL_CMD_BINARY");
-		ret = comp_data_blob_set_cmd(cd->model_handler, cdata);
+		ret = rtnr_set_bin_data(dev, cdata);


If you stop using comp_data_blob_set_cmd() for some cases and implement the message handling for those your self, you should do the same thing symmetrically for comp_data_blob_get_cmd(). One alternative would not to use the comp_data_blob_handler at all and write all message fragment handling your self.

jsarha · 2022-03-31T15:22:11Z

src/include/sof/audio/rtnr/rtklib/include/RTK_MA_API.h

+						unsigned int msg_index,
+						unsigned int num_elms,
+						unsigned int elems_remaining);
+


Using my PR #5509 you could use the new comp_data_blob_handler_new_ext() with single_blob = true and tap into alloc() and free() callbacks to return always the pointer to the static buffer (and return an error if the requested size is bigger than the buffer).

With this approach you should keep using the comp_data_blob_handler and friends mostly the way the other components do. The IPC fragment aggregating would be handled by data_blob.c functions. Just before processing call you should check with comp_is_current_data_blob_valid() if the configuration buffer is currently valid, or if it is in the middle of an update.

jsarha · 2022-03-31T16:15:26Z

src/audio/rtnr/rtnr.c

+	switch (cdata->data->type) {
+	case SOF_RTNR_CONFIG:
+		comp_dbg(dev, "rtnr_set_bin_data(): SOF_RTNR_CONFIG");
+		ret = comp_data_blob_set_cmd(cd->model_handler, cdata);


It won't work as such, since the handler only maintains one blob per com_data_blob_handler. I think the easiest would adding multiple handlers to the comp_data, one for each blob, and and select the correct one based on component specific type.

lgirdwood · 2022-04-19T15:50:24Z

@MingJenTai any update ?

sys-pt1s · 2022-04-20T06:32:51Z

Can one of the admins verify this patch?

MingJenTai · 2022-04-21T06:50:19Z

@MingJenTai any update ?

We haven't check #5509 yet. Will make the required modifications if necessary. Thanks!

cujomalainey · 2022-05-13T15:38:19Z

please rebase this PR or close it as it conflicts with main.

MingJenTai · 2022-05-16T01:37:30Z

Need to be refined. Close now.

MingJenTai requested review from lgirdwood, plbossart, mmaka1, lbetlej and dbaluta as code owners March 15, 2022 05:09

lgirdwood requested changes Mar 15, 2022

View reviewed changes

cujomalainey suggested changes Mar 15, 2022

View reviewed changes

MingJenTai force-pushed the main_pass_binary_data_to_RTNR branch 4 times, most recently from 658e003 to ed72088 Compare March 18, 2022 06:18

lgirdwood reviewed Mar 21, 2022

View reviewed changes

[RTNR] Pass binary data to RTNR

a8f3a89

This commit allows binary data to be passed to RTNR. There could be different types of the data, and those data will update the internal data of RTNR directly. Signed-off-by: Ming Jen Tai <[email protected]>

MingJenTai force-pushed the main_pass_binary_data_to_RTNR branch from ed72088 to a8f3a89 Compare March 23, 2022 08:49

lgirdwood reviewed Mar 30, 2022

View reviewed changes

jsarha reviewed Mar 31, 2022

View reviewed changes

MingJenTai closed this May 16, 2022

[RTNR] Pass binary data to RTNR #5544

[RTNR] Pass binary data to RTNR #5544

Conversation

MingJenTai commented Mar 15, 2022

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MingJenTai commented Mar 18, 2022

cujomalainey commented Mar 20, 2022 • edited Loading

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

lgirdwood Mar 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood commented Apr 19, 2022

sys-pt1s commented Apr 20, 2022

MingJenTai commented Apr 21, 2022

cujomalainey commented May 13, 2022

MingJenTai commented May 16, 2022

cujomalainey commented Mar 20, 2022 •

edited

Loading

lgirdwood Mar 30, 2022 •

edited

Loading